-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move Scalar to JS #935
Move Scalar to JS #935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice!
import { Field as InternalField } from './field.js'; | ||
import { Group as InternalGroup } from './group.js'; | ||
import { Scalar as ScalarBigint } from '../provable/curve-bigint.js'; | ||
import { mod } from '../bindings/crypto/finite_field.js'; | ||
import { Scalar } from './scalar.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add the function constructor for the Scalar
as well? I am a bit unsure - while it would be uniform, Scalar
is also not really a primitive like Field
or Bool
, especially since you cant prove operations like addition 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think the need is not pressing and requires refactoring the current constructor, so would save it for a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Exploring these files are going to be a great way for SnarkyJS devs to learn more about our crypto stack
return Scalar.from(z); | ||
} | ||
|
||
// TODO don't leak 'shifting' to the user and remove these methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
closes #925
bindings: o1-labs/o1js-bindings#30
This PR continues the work of #902 by moving the
Scalar
class from OCaml to JS. The OCaml class is removed entirely. Apart from minor API changes like introduction ofScalar.toConstant()
(useful for tests), this keeps the same API intact. The internal representation ofScalar
stays the same as before because theGroup
class relies on the structure and the PR to move that is in flight.Some other things we also did, which stick to the theme of cleaning up the OCaml side:
PrivateKey.to/fromBase58
to be the pure TS implementationLedger
module to theTest
module, because they are only used for tests nowsignFeePayer
method from the global exports and a couple of exposed functions that it depended on